Fix extension CLI teardown during AppHost build - #20021
Conversation
Route extension stop requests through cooperative cancellation and wait for in-flight pre-build cleanup before the CLI exits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20021Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 20021" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
The five-second cleanup timeout can still let a pending build outlive the CLI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Routes extension stop requests through cooperative CLI cancellation and waits for AppHost build cleanup.
Changes:
- Replaces immediate CLI termination with cancellation-manager signaling.
- Adds pending build cleanup handling and regression tests.
File summaries
| File | Description |
|---|---|
src/Aspire.Cli/Backchannel/ExtensionRpcTarget.cs |
Cooperatively cancels CLI execution. |
src/Aspire.Cli/Commands/RunCommand.cs |
Waits for canceled pre-build work. |
tests/Aspire.Cli.Tests/Backchannel/ExtensionBackchannelTests.cs |
Tests stop-request cancellation. |
tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs |
Tests cleanup ordering. |
tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs |
Supplies the cancellation manager. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Consolidate startup and pre-build cancellation into one helper with an explicit wait budget. Keep manager-owned shutdown under the central deadline, preserve bounded direct cancellation, and retain late task fault observation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Cancel synchronously at the fake project boundary and share the signaling time provider between command tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
James Newton-King (JamesNK)
left a comment
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
One lifecycle issue needs to be addressed before merge: extension cancellation after build completion can still let RunCommand finish before the pending AppHost run task has cleaned up. I confirmed it with a focused negative-control test and verified the minimal gate removal against the existing cancellation tests.
Proof: the PR's focused CLI classes pass (169 passed, 2 expected platform skips). The added post-build negative control fails on the PR head, then it and the three related cancellation tests pass after removing only the buildWaitCompleted gate. I did not rerun the full VS Code dynamic-debug E2E shard; that remains the real-path proof gap.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Cancellation during the startup-timeout cleanup wait can still allow the CLI to exit before child-process cleanup completes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
Propagate cancellation during timeout cleanup and centralize draining in finally so linked-token cancellation and concurrent RPC failures cannot bypass it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
There was a problem hiding this comment.
🟡 Changes recommended
The final cleanup decision has a race that can still let manager-triggered cancellation complete before the pending AppHost run is drained.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
Fence cancellation forwarding before the final drain decision, preserve teardown failure cleanup, and upgrade earlier bounded waits for a late manager stop. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Tests selector2 / 99 PR test projects · 4 PR jobs · 1 advisory-only target, from 8 changed files. Selected PR test projects (2 / 99)
Selected PR jobs (4)
Advisory workflow impact (1)
How these were chosen — grouped by what changed📦 affected project 🧪 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
James Newton-King (JamesNK)
left a comment
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
Reviewed the CLI cancellation and teardown changes. No issues found.
|
✅ No documentation update needed. Step 5 branch taken: Triggered signals (1): Why no docs change is needed: This PR is an internal reliability bug fix, not a new user-facing feature or behavior change. Previously, the VS Code extension's Docs already describe this behavior by name, in
The fix simply ensures the VS Code extension's stop request now goes through this already-documented three-step sequence instead of bypassing it — the observable, documented end-user behavior of (summary truncated) |
|
The CI build failed due to test failure(s) that appear unrelated to the PR changes. These may be flaky tests. Suspected flaky test(s): Suggested actions:
You can re-run the failed jobs from the workflow run page. |
Description
Stopping a dynamic debug session could terminate the Aspire CLI while a cold single-file AppHost build was still running. The extension's
stopCliRPC used immediate process exit, bypassing normal command cancellation and child-process cleanup. On Windows, the orphaneddotnet buildprocess retained the fixture workspace as its working directory, so E2E teardown failed withEBUSYwhile removing the second dynamic-debug workspace.This change routes extension stop requests through the shared
ConsoleCancellationManager, allowing the CLI's existing cancellation and process-tree cleanup paths to run.RunCommandalso keeps pending pre-build work owned by the active command handler during manager-triggered shutdown. That is important because the cancellation manager's graceful deadline and final drain are the process-level escape hatch; returning from the handler after a separate local timeout could otherwise make shutdown appear complete before nested build cleanup finished.Direct callers that cancel
RunCommandwith an unrelated token still use the bounded five-second wait. Those invocations do not have the cancellation manager's process-level deadline, so waiting indefinitely there could hang embedded or test callers.The manager-owned cancellation regression requests the stop RPC synchronously from the fake project's
RunAsynccallback. This ensures the command enters its cleanup wait before fake time advances, rather than relying onTask.Yield()to schedule the handler. The pending run is released and awaited infinally, and the signaling time provider is shared withAppHostLauncherTests.Validation:
RunCommandTests,AppHostLauncherTests, andExtensionBackchannelTests(one Unix-only case and one case requiring Windows symlink privileges).dynamic-debug-configurationshard passed three complete runs (9/9 tests), including validation with a temporary delayed AppHost build that reproduced the directory lock before the fix. The follow-up changes only refine test coverage and share its helper.Checklist
<remarks />and<code />elements on your triple slash comments?